chore: api http router improvements#38227
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughRequest parsing now populates Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Router as Router (Hono)
participant Parser as parseBodyParams
participant LoggerMW as Logger Middleware
participant LogPayload as getRestPayload
participant Handler as API Handler
Client->>Router: HTTP request
Router->>Parser: parse query/body -> c.set('queryParams','bodyParams')
Router->>LoggerMW: invoke middleware with context
LoggerMW->>LogPayload: await getRestPayload(c.req)
LogPayload-->>LoggerMW: payload or null
LoggerMW-->>Router: continue to handler
Router->>Handler: construct APIActionContext (route, incoming, queryParams, bodyParams)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
12a4f8f to
b1b33d3
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38227 +/- ##
===========================================
- Coverage 70.41% 70.38% -0.03%
===========================================
Files 3161 3161
Lines 110653 110650 -3
Branches 19827 19866 +39
===========================================
- Hits 77911 77881 -30
- Misses 30719 30741 +22
- Partials 2023 2028 +5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
dd78dad to
536fd45
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rest-typings/src/v1/omnichannel.ts (1)
4502-4505: Type guard does not match schema nullability forcustomFieldId.The schema defines
customFieldIdwithnullable: trueand norequiredarray (making it optional), but the type guard narrows tocustomFieldId: string. After validation,customFieldIdcould benullor omitted (undefined), violating the type guard. Align the generic type tocustomFieldId?: string | nullto match the schema.Suggested fix
export const isPOSTLivechatSaveCustomFieldsParams = ajv.compile<{ - customFieldId: string; + customFieldId?: string | null; customFieldData: Omit<ILivechatCustomField, '_id' | '_updatedAt'> & { field: string }; }>(POSTLivechatSaveCustomFieldsSchema);
🤖 Fix all issues with AI agents
In `@packages/rest-typings/src/v1/omnichannel.ts`:
- Around line 4446-4457: Update the regex character classes to move the hyphen
to the end so they read `[0-9a-zA-Z_-]` for both customFieldId and
customFieldData.field to avoid creating an unintended range; also reconcile the
nullable mismatch by either removing `nullable: true` from the `customFieldId`
schema or updating the type guard that references `customFieldId` (the check
near the type guard around line ~4502) to accept `string | null` so schema and
runtime types match.
🧹 Nitpick comments (1)
packages/http-router/src/Router.ts (1)
157-175: AlignbodyParamstyping with possible JSON shapes.
request.raw.clone().json()can return arrays or primitives, but the context types areRecord<string, unknown>. Consider normalizing non‑object bodies to{}or widening the type so downstream code doesn’t rely on an incorrect shape.
536fd45 to
2186968
Compare
|
While reviewing the Created CORE-1741 to track this separately. |
|
@ricardogarim are they really broken? I've found this test to validate it (should) be working: Rocket.Chat/apps/meteor/tests/end-to-end/api/incoming-integrations.ts Lines 343 to 366 in 0580cfc |
|
@d-gubert maybe the Livechat changes could be in another PR? they look like regular changes not necessarily related to "api http router improments" |
We need to modify that endpoint, otherwise tests fail because of this #38227 (comment) I can make a much more contrived change just to make sure the tests are passing, then open a new PR with the appropriate changes to schemas and all. But the endpoint can't go unscathed 😛 |
|
Forgot to mention @sampaiodiego #38227 (comment) 😬 |
2186968 to
5ffad11
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/models/src/models/LivechatCustomField.ts (1)
52-75: Return value missing_idin update path.When updating an existing record (
_idis truthy), the returnedrecordobject lacks the_idproperty, which violates theILivechatCustomFieldreturn type. The insert path correctly assignsrecord._id = field, but the update path does not preserve the ID on the returned object.🐛 Proposed fix
async createOrUpdateCustomField( _id: string | null, field: string, label: ILivechatCustomField['label'], scope: ILivechatCustomField['scope'], visibility: ILivechatCustomField['visibility'], extraData: any, ) { const record = { label, scope, visibility, ...extraData, }; if (_id) { await this.updateOne({ _id }, { $set: record }); + record._id = _id; } else { record._id = field; await this.insertOne(record); } return record; }
🤖 Fix all issues with AI agents
In `@apps/meteor/app/api/server/router.ts`:
- Around line 44-49: The APIActionContext construction should defensively
default potentially-undefined context entries; update the code that builds
APIActionContext (the object with requestIp, urlParams, queryParams, bodyParams,
request) to use c.get('bodyParams') ?? {} and c.get('queryParams') ?? {} so
bodyParams and queryParams are always objects even if c.get(...) returns
undefined, keeping the existing fields (e.g., requestIp: c.get('remoteAddress')
and urlParams: req.param()) unchanged.
3ad9b41 to
5ffad11
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/meteor/app/api/server/middlewares/logger.ts`:
- Around line 23-25: The logger middleware currently uses redact: ['password']
which won't match nested fields inside the request body; update the redact
configuration to target the nested payload key (use redact:
['payload.password']) in the logger/middleware configuration so that any
password inside the payload object is properly redacted; locate the redact
property in the logger middleware file
(apps/meteor/app/api/server/middlewares/logger.ts) and replace the array entry
'password' with 'payload.password'.
In `@packages/http-router/src/Router.ts`:
- Around line 165-168: parseBodyParams currently returns form entries directly
from request.raw.clone().formData() (in Router.ts) so payload fields sent as
application/x-www-form-urlencoded remain JSON strings; update the code in
parseBodyParams to detect a "payload" entry, attempt to JSON.parse(payload) and
merge/replace its parsed object into the returned params (e.g., const parsed =
JSON.parse(payload) and return { ...otherParams, ...parsed }), and ensure
parsing is wrapped in try/catch so malformed JSON falls back to the original
string; this change will satisfy the incoming-integrations test that sends
payload=JSON.stringify(payload) and allow processWebhookMessage to receive
unwrapped fields like msg/text.
🧹 Nitpick comments (1)
apps/meteor/server/lib/logger/logPayloads.ts (1)
34-39: JSON parse failure returns{ payload: null }instead ofnull.When JSON parsing fails, the function returns
{ payload: null }, which will spread into the log object aspayload: null. This differs from the disabled case (line 28) which returnsnulland spreads to nothing. Consider returningnullon parse failure to avoid logging apayload: nullfield for unparseable bodies.🔧 Suggested fix
return { payload: await request.raw .clone() .json() - .catch(() => null), + .catch(() => undefined), };Or alternatively, to match the disabled behavior:
- return { - payload: await request.raw - .clone() - .json() - .catch(() => null), - }; + try { + return { payload: await request.raw.clone().json() }; + } catch { + return null; + }
e8e1db7 to
08e89d1
Compare
20ecbe8 to
00a2cbe
Compare
00a2cbe to
d9239a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/server/lib/logger/logPayloads.ts`:
- Around line 26-39: getRestPayload currently only catches errors from
request.raw.clone().json()'s .json() call but not from request.raw.clone()
itself, which can throw if the body was already consumed; modify the async
branch in getRestPayload to wrap the entire clone+json sequence in a try/catch
(surround request.raw.clone().json() with try) and on any error return {
payload: null } (or { payload: '[multipart/form-data]' } earlier), ensuring
clone failures are handled and logging remains best‑effort.
🧹 Nitpick comments (1)
packages/http-router/src/Router.ts (1)
171-174: Consider debug-level logging for parse failures.The silent catch makes debugging harder when requests fail unexpectedly. While the design allows endpoints to handle their own parsing, a debug-level log would help trace issues without affecting behavior.
🔧 Optional: Add debug logging
- } catch { - // No problem if there is error, just means the endpoint is going to have to parse the body itself if necessary + } catch (e) { + logger.debug({ msg: 'Body parsing skipped, endpoint may handle manually', err: e }); return {}; }
5533271 to
c6119ad
Compare
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/rest-typings/src/v1/omnichannel.ts">
<violation number="1" location="packages/rest-typings/src/v1/omnichannel.ts:4503">
P2: The type for `customFieldId` no longer matches the schema: the schema allows `null` and omission, but the type now requires a key and disallows `null`. This will reject valid inputs in typing/compile-time usage. Consider making the property optional and allowing `null` to align with the schema.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
34a3e12 to
ea2565c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/app/livechat/server/api/v1/customField.ts`:
- Around line 114-123: The validation order is inverted: the regex check for
customFieldId should run before querying the DB so format errors return
immediately; update the block that uses customFieldId (the conditional
containing LivechatCustomField.findOneById and the /^[0-9a-zA-Z-_]+$/ test) to
first test the regex and return API.v1.failure('Invalid custom field name...')
on failure, then call LivechatCustomField.findOneById(customFieldId) and return
API.v1.failure('Custom Field Not found') if that lookup returns null.
Proposed changes (including videos or screenshots)
parseBodyParamsandparseQueryParamsinRocketChatAPIRouterpasswordfield when logging requests' payloadsIssue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Security
Refactor
Bug Fixes